The pattern matching here is cool, I think it could even be useful to get it in as a separate PR - maybe with a couple of uses in desugaring to test out the benefits. (I assume it's a zero cost abstraction? It certainly seems like it can be.)

I also think it's excellent to have an executable specification of what's valid input AST. I love this.

However, I do have some design misgivings about the details - it feels rather like a repetition of large parts of the matching code within desugaring, even though in a nicely condensed form. Also it contains (IIUC?) random pieces of logic which are currently in other passes, such as const not being valid in local scopes and global not being valid in value position. Those things seem to require correctly modeling some parts of the later passes of lowering but on the non-simplified surface AST. In the past, we've found that to be bug prone (see the many bugs which resulted from trying to model hygiene in vars-introduced-by-patterns in macroexpand.scm). Perhaps that's not a problem if we limit the types of validation we're doing to things which are "easy enough" to do on the surface AST, but it does make me worry we can end up in a similar situation.

I like that you've put proper error reporting in here. That seems really important to make this practical for end users rather than macro authors - keeping in mind that non-macro-authors are the vast majority of Julia users. For error reporting I think we need the principle that "if the parser can produce a form, we should assume an end user will see the error", and go to the effort of trying to write good messages. With that in mind, there's a fair few cases here where I think the errors got worse because we now talk about the names of Kinds. I do think we should avoid that where possible when expressions are likely to be generated directly from the source code. A particular example is the expression [a, b; c] which is extremely likely to be typed by a user given that [a,b] and [a b; c] are both valid code. In general, the distinction of "end user vs macro author" is the distinction I currently give in desugaring between writing a custom message vs just using @chk without any special message.

In the current state, I'm feeling this is ok, but also I feel like it doesn't hit satisfying tradeoffs in terms of avoiding duplication with the rest of lowering. However, I have a few thoughts about future design/implementation directions which might get us from "eh that's implicitly a lot of code duplication" to "wow this was a great improvement":

1. As an executable specification, can we use this pattern matching to drive desugaring itself? This would alleviate a lot of my concerns about duplicating this logic and about having two different specifications to maintain and synchronize (one within the validation code and one implicit in the recursion of desugaring).
2. Can we use it to produce a "`K"error"-poisoned but structurally valid" AST, such that desugaring can run easily over that AST, allowing us to implement Error propagation via AST #29 without tons of custom matching?

Even better if we can combine both of these ideas.

You understand far better than me how lowering should work, so my comments might not be super helpful. But with that disclaimer, I'd still like to share my thoughts.

I think the basic idea of this PR is to separate validation from transformation and have the validation act as a readable specification. But the implementation still ends up being recursive, and from a general user's perspective, I think this validation (even if it looks much easier to read than desugaring, probably thanks to the beautiful pattern matching system) would still be hard to use directly as a specification? In many cases, users will probably still rely on error messages.

As an executable specification, can we use this pattern matching to drive desugaring itself?

So I agree with @c42f's point above.
From an implementation perspective, separating validation and transformation seems to lead to unavoidable code duplication (even if pattern matching abstracts most of it, structural duplication might still be there AFAIU). So, merging them might be better in terms of implementation simplicity. Even if we merge them, the pattern matching syntax from this PR could still greatly improve readability. (Maybe if we use agents nicely, we could even generate a human-readable specification from a more readable desugaring implementation? IDK)

Also, if we think about implementing Claire's point (2), we'd likely need to make use of validation information during transformation. So, based on a rough implementation sketch I can think of right now, doing both in one place would probably result in a simpler implementation.

But other points @mlechu brought up might need more thought.

The checking here is especially unwanted given that much of desugaring's input is output from other desugaring functions (related to (4) below), which shouldn't go through the same level of checking as arbitrary user input.

Regarding this, my understanding is that doing validation first can help avoid unnecessary validation when recursive transformations happen in the transformation phase. From a performance perspective, though, I don't think the order of nodes explored will change, so there might not be a huge performance difference. Or maybe code generated by macros needs fundamentally different validation/transformation than regular source input? I do think we need to improve validation messages for invalid code generated from macro contexts. JL already keeps the macro expansion context, so I believe we can use that to dispatch appropriate messages even with a one-path validation implementation.

How each expression gets through desugaring is generally unknown.

This point is very interesting. Does avoiding this problem mean we need to do extensive validation as a separate pass first? The simplest (naivest) solution I can think of is to add an attribute to nodes that have already been desugared and use that flag to decide if desugaring is still needed. However, we might need to think about how this changes the complexity compared to doing validation as a separate pass first.

(I assume it's a zero cost abstraction? It certainly seems like it can be.)

Pattern-matching can be faster than the equivalent if chain with a smarter implementation! Right now, @stm produces the easiest possible expansion though.

In the current state, I'm feeling this is ok, but also I feel like it doesn't hit satisfying tradeoffs in terms of avoiding duplication with the rest of lowering.

To be clear, my intention with this PR is to delete more than its weight in desugaring code. As satisfying as that would be now, we're actively trying to achieve parity with flisp, so I want to wait before doing that.

1. As an executable specification, can we use this pattern matching to drive desugaring itself? This would alleviate a lot of my concerns about duplicating this logic and about having two different specifications to maintain and synchronize (one within the validation code and one implicit in the recursion of desugaring).

I may have failed to argue my point. I don't think validation and desugaring should happen in the same pass. Desugaring often needs to eat its own output, which doesn't need to follow the rules of its input, and doesn't need to go through validation.

Squeezing desugaring into the framework in validation.jl would also be much more painful than bringing @stm and invariants from validation into desugaring piece by piece.

I don't think desugaring should be an AST specification; the aim is to have one AST specification that macro authors follow (or see errors about) and desugaring gets to assume its inputs follow. This isn't the same as keeping two specifications synchronized. If desugaring handles a superset of the specification, we don't need to care; the validator has run. This way, we don't get the "how did this fall through lowering?" or "macro author found a clever trick: produce a lowering-internal form" situations we know and love from flisp.

There would be code duplication with this approach, but the current state is the most duplicated it can be (all destructuring and error checking implemented twice). Things will only shrink from here, especially if we manage to simplify the AST and desugaring.

2. Can we use it to produce a "`K"error"-poisoned but structurally valid" AST, such that desugaring can run easily over that AST, allowing us to implement Error propagation via AST #29 without tons of custom matching?

I don't think we should do this, and would at least like to wait before trying it out. The benefit of partial lowering doesn't seem worth the cost of every node now being valid or an error node, which sounds like "two cases" but actually means "two cases, per place in the SyntaxTree, per lowering pass." I fear it would be like Expr(:escape) wrappers in expr_to_syntaxtree or LineNumberNode in macros, only less polite about where it appears and much less clear what the semantics of it are.

For error reporting I think we need the principle that "if the parser can produce a form, we should assume an end user will see the error", and go to the effort of trying to write good messages.

Agree; some thoughts are in this conversation. I still need to bring some validation.jl errors up to baseline, but the goal is for valid_st1 to be our tool to produce fancier errors in the future.

I may have failed to argue my point. I don't think validation and desugaring should happen in the same pass. Desugaring often needs to eat its own output, which doesn't need to follow the rules of its input, and doesn't need to go through validation.

I'm actually not arguing that validation and desugaring should happen in the same pass :-)

Instead, what I'm hoping/dreaming of is using one canonical set of patterns to drive both desugaring and validation.

For example, matching all the variants of function signatures is an annoying piece of code which both validation and desugaring need to do. It would be wonderful if they could both share the same matching code. The tricky thing about this vague dream is that matching comes in two parts (1) the pattern specification and (2) the actions to execute when patterns match. The patterns might be shared, but the actions very much are not. The big design unknown with this idea is whether there's a convenient way to factor the actions out separately. If not, the idea is doomed.

Desugaring often needs to eat its own output

It currently does, though I feel this is not ideal. It might be feasible to stop doing this and there's a few ideas we could try. But it's definitely a bunch of work. Some ideas:

• Use different kinds internally within lowering to the kinds of the input. (Downside: a proliferation of kinds with almost the same semantics). The idea of using an attribute for these instead could possibly help, but there's some complexity associated with that as well.
• Define helper functions with restricted expansion semantics, and pass pieces of AST to them rather than generating a big blob of new AST and passing that to the outer expand_forms_2. For example, if we currently generate a new assignment lhs = rhs and call generic expand_forms_2 on that, maybe we should instead pass it to a slightly refactored expand_assignment(ctx, srcref, lhs, rhs) rather than making a new AST and passing it to the outer expansion function.

At the outset I favor the second of these ideas but it could be a lot of refactoring. Would need some prototyping to test how feasible it is.

I fear it would be like Expr(:escape) wrappers in expr_to_syntaxtree or LineNumberNode in macros, only less polite about where it appears and much less clear what the semantics of it are.

Right, so addressing this concern is what I mean by "structurally valid" in "K"error"-poisoned but structurally valid". The goal is to enforce "enough" structural validity in the AST up front precisely so that future passes can always match the structures they expect. For example, if the input AST is [K"=" lhs] (ie, missing the rhs) and we expand this into [K"=" lhs [K"error"]], desugaring would be able to run without issues. My intuition is that desugaring would still need minor adjustments but they wouldn't be intrusive.

Instead, what I'm hoping/dreaming of is using one canonical set of patterns to drive both desugaring and validation.

I see, and agree this would be nice if a convenient factoring exists.

Define helper functions with restricted expansion semantics, and pass pieces of AST to them rather than generating a big blob of new AST and passing that to the outer expand_forms_2

This is the goal! I think knowing what our input looks like will help. Producing surface AST and re-desugaring is convenient, but we could probably take a more direct route through desugaring in some places.

Knowing what desugaring's output looks like would also help, but doesn't feel critical given that it's only a problem for us, and not macro authors. If we do end up wanting a similar tree specification for intermediate trees, it would make sense to take the alternate simple-errors approach from above and only run it in testing.

For example, if the input AST is [K"=" lhs] (ie, missing the rhs) and we expand this into [K"=" lhs [K"error"]], desugaring would be able to run without issues. My intuition is that desugaring would still need minor adjustments but they wouldn't be intrusive.

If we receive a tree [K"=" something], we can't assume something was meant to be on the left or right. I think lowering should just throw an error.

If you mean to only allow K"error" in generic-statement-or-value position (i.e. not a function name, foreigncall name, assignment lhs, loop iteration spec, etc), I agree it wouldn't be as disruptive to further passes. I'd still prefer "partial lowering" be the responsibility of the consumer of it, with our responsibility being to either figure out where the AST is broken, or produce correct output (both still WIP).